[mypyc] feat: support constant folding in translate_index_expr [1/1]
#19972
+6
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
JukkaL
reviewed
Oct 2, 2025
mypyc/irbuild/expression.py
Outdated
| if ( | ||
| isinstance(base.type, RTuple) | ||
| and isinstance(folded_index := constant_fold_expr(builder, index), int) | ||
| and folded_index >= 0 |
There was a problem hiding this comment.
Check for index overflow (it may generate an error elsewhere, but we don't want to crash the compiler if it's ignored).
There was a problem hiding this comment.
Done.
I also just thought about reverse indexing, I'll implement that too
There was a problem hiding this comment.
We will need to adapt TupleGet before we can pass negative values here.
I've implemented that in a separate PR #19990 , lets pause this one until we get that merged in so we don't have to come back to this section later
maybe I should just compute the correct positive index value when we encounter a negative index, rather than doing this? Where's the best place to do that? Should that be with this or in a separate PR?
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
BobTheBuidler
added a commit
to BobTheBuidler/mypy
that referenced
this pull request
Oct 2, 2025
This PR modifies `TupleGet.__init__` to automatically convert negative indexes to positive indexes instead of crashing at the assert This won't change functionality on its own, but will allow us to pass negative values in python#19972 so I think we should consider this PR a prerequisite to that one
BobTheBuidler
added a commit
to BobTheBuidler/mypy
that referenced
this pull request
Oct 2, 2025
This PR modifies `TupleGet.__init__` to automatically convert negative indexes to positive indexes instead of crashing at the assert This won't change functionality on its own, since none of the existing calling locations can pass a negative value, but will allow us to pass negative values in python#19972 so I think we should consider this PR a prerequisite to that one
This comment was marked as off-topic.
This comment was marked as off-topic.
p-sawicki
added a commit
that referenced
this pull request
Oct 7, 2025
This PR modifies `TupleGet.__init__` to automatically convert negative indexes to positive indexes instead of crashing at the assert This won't change functionality on its own, since none of the existing calling locations can pass a negative value, but will allow us to pass negative values in #19972 so I think we should consider this PR a prerequisite to that one --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Piotr Sawicki <[email protected]>
|
Now that #19990 is merged, this PR is ready for review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR attempts to constant fold the index value in
translate_index_exprI'm not sure any test changes are warranted for a small PR of this nature.